Skip to content

[OCTRL-1088] Environment Controller#816

Merged
justonedev1 merged 3 commits into
masterfrom
OCTRL-1088
May 26, 2026
Merged

[OCTRL-1088] Environment Controller#816
justonedev1 merged 3 commits into
masterfrom
OCTRL-1088

Conversation

@justonedev1
Copy link
Copy Markdown
Collaborator

@justonedev1 justonedev1 commented May 11, 2026

Apart from environment controller I also created separate binaries for environment manager and task manager, which is the cause of most of the yaml piping.

@justonedev1 justonedev1 force-pushed the OCTRL-1088 branch 3 times, most recently from 53d5a96 to 913d6af Compare May 11, 2026 13:53
@justonedev1 justonedev1 marked this pull request as ready for review May 11, 2026 14:51
@justonedev1 justonedev1 requested a review from knopers8 as a code owner May 11, 2026 14:51
@justonedev1 justonedev1 changed the title [WIP] [OCTRL-1088] [OCTRL-1088] May 11, 2026
knopers8
knopers8 previously approved these changes May 20, 2026
Copy link
Copy Markdown
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I took some time to get acquainted with the news and finally tried running it today - it works!

I have some minor questions, but it can be merged already. Please put a human-readable title to the PR though.

"strings"

pb "github.com/AliceO2Group/ControlOperator/internal/controller/protos/generated"
pb "github.com/AliceO2Group/Control/operator/internal/controller/protos/generated"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, isn't it control-operator?

Suggested change
pb "github.com/AliceO2Group/Control/operator/internal/controller/protos/generated"
pb "github.com/AliceO2Group/Control/control-operator/internal/controller/protos/generated"

Copy link
Copy Markdown
Collaborator Author

@justonedev1 justonedev1 May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct, I will address it in separate PR as proper renaming would touch multiple files and I can do at least once something properly in it's own PR/commit :D

task.Spec.Control = *template.Spec.Control.DeepCopy()

if foundIdx := slices.IndexFunc(taskReference.Env, func(envVar v1.EnvVar) bool { return envVar.Name == "OCC_CONTROL_PORT" }); foundIdx == -1 {
log.Error(fmt.Errorf("didn't find OCC_CONTROL_PORT in env"), "failed to fill in env vars from template")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we return if there is error?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, to have symetrical behavior with the other error handling in the controller... However I am not sure that it is the correct behavior here, as it drops the deployment, which might not be desired outcome. I added TODO into the code to address this potentialy

@justonedev1
Copy link
Copy Markdown
Collaborator Author

justonedev1 commented May 26, 2026

You mentioned in person that environment controller fails to relay transition arguments on your setup. When I tried on your minikube setup it indeed failed to configure tasks. However the same problem appeared when I run tasks individually without environment (using eg. readout-test.yaml) so the issue is not with environment-controller. Everything works on my setup. I didn't debug further for now as the issues on the minikube setup are not relevant for this PR.

@justonedev1 justonedev1 changed the title [OCTRL-1088] [OCTRL-1088] Environment Controller May 26, 2026
@justonedev1 justonedev1 merged commit 81a1fca into master May 26, 2026
4 checks passed
@justonedev1 justonedev1 deleted the OCTRL-1088 branch May 26, 2026 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants